Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better node display #137

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

benwr
Copy link
Contributor

@benwr benwr commented Jun 3, 2023

Change the Display instance for Node. Previously it printed a simple pre-order traversal, which was sometimes ambiguous because e.g. for nested n-ary functions it wasn't clear which things were arguments.

Now, this should produce a visual that is (a) unique, and (b) can be parsed back into an equivalent Node.

Copy link
Owner

@ISibboI ISibboI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you a lot, this looks great! Can you add some tests with some simple and more complicated expressions whose Display output parses into the same operator tree?

src/tree/display.rs Show resolved Hide resolved
@benwr
Copy link
Contributor Author

benwr commented Jun 4, 2023

How would you feel about adding a dev dependency on quickcheck? I might want to randomly generate operator trees and check that they round-trip to the same trees.

@ISibboI
Copy link
Owner

ISibboI commented Jun 8, 2023

I am totally fine with new dev dependencies.

@ISibboI ISibboI force-pushed the better_node_display_rebased branch from b56a199 to 3593b8f Compare June 17, 2023 09:53
ISibboI
ISibboI previously approved these changes Jun 27, 2023
@ISibboI
Copy link
Owner

ISibboI commented Jun 27, 2023

It seems like propagating the precision does not work. We can either just not support floating point precision, or we fix that somehow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants